Avoid prepend accumulation problems #3474
Conversation
| initializer 'spree.decorators' do |app| | ||
| # Ensure decorated constants are unloaded before reloading decorators | ||
| config.to_prepare_blocks.unshift -> { ActiveSupport::Dependencies.clear } |
There was a problem hiding this comment.
Will this clear all dependencies declared in all to_prepare blocks of the application? What if the app or other gems added dependencies? Could this be breaking?
There was a problem hiding this comment.
@tvdeyen that's why I ended up putting it at the beginning with unshift, anyway, although it helps with warnings about redefined constants it doesn't help with the initial problem I was trying to solve.
Digging deeper I've found that when a constant is loaded during the initialization (e.g. inside config/initializers/_spree.rb) it won't be reloaded properly and any decorator will end up being prepended at each reload. Aside from that was also evident that to_prepare is called twice during reloads, only aggravating the problem.
Right now I'm still experimenting but a partial solution seem to involve a combo of an initializer block plus app.reloader.to_run, this solves a lot and relies on the call to ActiveSupport::Dependencies.clear that rails already does within the reload cycle, but still doens't solve the problem for constants loaded during initialization.
I'll make sure to mark the PR as ready when I'm more confident that the solution is proven
There was a problem hiding this comment.
Thank you so much for looking into this and the detailed explanation. 💯
Loading decorators within `to_prepare` with `prepend` or `include` ends up adding multiple copies of the decorator to the ancestors of the decorated class. This typically causes trouble when using `super` within methods or when changes done in `prepended` or `included` are not idempotent. Clearing dependencies also solves problems related to overwriting constants and removed methods (i.e. when a method is removed by a decorator, previous instances of the decorator that are still in the ancestors list will still have that method defined and will intercept calls to it).
elia
force-pushed
the
elia/avoid-prepend-accumulation-problems
branch
from
c582817 to
c588e40
Compare
|
I think this problem is similar to the one we faced on extensions on solidus_support. We ended up with this module that handle it. The trick was to add the decorator folders to the Rails TL;DR: We should suggest using a better directory structure for decorators (whose content match module/class names) for example: # app/decorators/models/my_app/spree/product/add_a_feature.rb
# which defines
module MyApp
module Spree
module Product
module AddAFeature
# super cool code here
::Spree::Product.predend self
end
end
end
endand change this PR in something like: application <<-RUBY
decorators_path = root.join("lib/decorators/#{engine}")
config.autoload_paths += decorators_path.glob('*')
initializer 'spree.decorators' do |app|
# Load application's decorators
config.to_prepare do
decorators_path.glob('**/*.rb') do |decorator_path|
require_dependency(decorator_path)
end
end
end
RUBY |
|
The code for decorator loading is no longer generated, so we can close it. |
Description
Loading decorators within
to_preparewithprependorincludeends up adding multiple copies of the decorator to the ancestors of
the decorated class. This typically causes trouble when using
superwithin methods or when changes done in
prependedorincludedarenot idempotent.
Clearing dependencies also solves problems related to overwriting
constants and removed methods (i.e. when a method is removed by a
decorator, previous instances of the decorator that are still in the
ancestors list will still have that method defined and will intercept
calls to it).
Checklist: